feat(databases): add --catalog flag to databases create#125
Conversation
Maps to the default_catalog field on POST /v1/databases, separating the human-readable --name from the SQL catalog alias used in queries. Also fixes config tests that failed when HOTDATA_API_KEY was set in the environment by unsetting it inside the with_temp_config_dir test helper.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if let Some(c) = catalog { | ||
| req.insert( | ||
| "default_catalog".to_string(), | ||
| serde_json::Value::String(c.to_string()), | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: the new default_catalog request branch has no test coverage. Every create_database_request test call passes None for the catalog arg, so this insert is never exercised. Consider adding a test like create_database_request(None, Some("analytics"), "public", &[], None) asserting req["default_catalog"] == "analytics". (not blocking)
| let catalog = result.default_catalog.as_deref() | ||
| .or(result.name.as_deref()) | ||
| .unwrap_or("default"); |
There was a problem hiding this comment.
nit: now that name is a free-form display string (e.g. "Sales reporting"), this fallback can put an invalid catalog into the query hint. If the server returns a name but no default_catalog, the hint becomes SELECT * FROM Sales reporting.public.<table> — which won't parse. Falling back to "default" (or omitting the hint) would be safer than falling back to the display name.
Same conflation still exists in get (line 435, let catalog = db.name.as_deref()... for sql_prefix) and the list empty-state hint (line 396, --name <catalog_name>), which now read as stale given this PR's name/catalog split. Worth a follow-up if DatabaseInfo can surface default_catalog. (not blocking)
There was a problem hiding this comment.
LGTM. The --catalog flag is wired through cleanly and the request/response changes are correct. The two outstanding nits (untested default_catalog branch, and the name-as-catalog fallback in the query hint / get / list empty-state) are already captured in existing threads and remain non-blocking.
…n all output - Add `default_catalog` field to `Database` struct so `databases show` and table commands use the correct SQL alias instead of falling back to the display name - Use default_catalog (falling back to name, then "default") in databases show sql_prefix, tables list full_name, tables load, and tables delete output - Fix --catalog arg doc: remove stale 24h expiry claim - Update README and skills/hotdata/SKILL.md to document --name as display name and --catalog as the SQL alias, with corrected examples
There was a problem hiding this comment.
Reviewed the --catalog flag addition. The flag is wired correctly end-to-end (command.rs → main.rs → databases::create → create_database_request), all call sites and tests are updated for the new arg, and the default_catalog field round-trips through both Database and CreateDatabaseResponse. The config test fix (clearing HOTDATA_API_KEY) is sound. The two existing nit threads (test coverage for the default_catalog request branch; display-name fallback in query hints) are non-blocking. LGTM.
1 participant
Summary
--catalogflag todatabases create, mapping todefault_catalogonPOST /v1/databases--name(display name) from the SQL catalog alias used in queries (SELECT * FROM <catalog>.schema.table)CreateDatabaseResponsenow deserializesdefault_catalogand displays it in table outputHOTDATA_API_KEYwas set in the environmentUsage
hotdata databases create --name "Sales reporting" --catalog analyticsTest plan
databases create --name "..." --catalog <alias>— API receivesdefault_catalog, response round-trips, table output showscatalog:line and query hint uses catalog namedatabases create --name "..."(no catalog) — unchanged behaviourdatabases create --catalog <alias>(no name) — works, name shown as nullHOTDATA_API_KEYset